test: add fork tests for upgradable contracts #594
Conversation
s2imonovic
changed the title
📝 WalkthroughWalkthroughIntroduces a unified fork testing framework (BaseForkTest) for standardized multi-chain proxy upgrade testing, replacing three legacy simulation scripts. Adds concrete test implementations for ERC20Custody, GatewayEVM, and GatewayZEVM with pre/post-upgrade state verification across multiple chains. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Fork Test
participant Fork as Forge Fork
participant Proxy as Proxy Contract
participant Admin as Admin Account
Test->>Fork: setUp() - create forks for all chains
activate Fork
Fork-->>Test: fork IDs returned
deactivate Fork
Test->>Test: testUpgradeGatewayOnAllChains()
loop for each ChainConfig
Test->>Fork: selectFork(forkId)
Test->>Proxy: Read current implementation
Test->>Proxy: Capture pre-upgrade state
Test->>Test: Deploy new implementation
Test->>Admin: vm.startPrank(adminAddress)
Admin->>Proxy: upgradeToAndCall(newImpl, data)
Test->>Proxy: Verify new implementation
Test->>Proxy: Assert state preservation
Test->>Admin: vm.stopPrank()
Test->>Test: Log upgrade success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
test/fork/BaseForkTest.sol(1 hunks)test/fork/ERC20CustodyFork.t.sol(1 hunks)test/fork/GatewayEVMFork.t.sol(1 hunks)test/fork/GatewayZEVMFork.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/fork/GatewayEVMFork.t.soltest/fork/ERC20CustodyFork.t.soltest/fork/GatewayZEVMFork.t.soltest/fork/BaseForkTest.sol
🪛 GitHub Actions: Test
test/fork/GatewayEVMFork.t.sol
[error] 1-1: vm.envString: environment variable "ETHEREUM_RPC_URL" not found
test/fork/ERC20CustodyFork.t.sol
[error] 1-1: vm.envString: environment variable "ETHEREUM_RPC_URL" not found
test/fork/GatewayZEVMFork.t.sol
[error] 1-1: vm.envString: environment variable "ETHEREUM_RPC_URL" not found
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: slither
- GitHub Check: generate
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/fork/BaseForkTest.sol (1)
9-15: Guard missing RPC env vars to prevent CI failures.This issue was already flagged in a previous review:
vm.envStringreverts when the environment variable is unset, causing CI failures when RPC URLs are missing. Usevm.envOrwith a default value or checkvm.envExistsfirst, and updatesetUp()to gracefully skip or bail out when required RPC URLs are unavailable.
🧹 Nitpick comments (3)
test/fork/BaseForkTest.sol (3)
59-66: PreferassertNotEqoverassertfor better test output.Using
assertprovides minimal information on failure. Replace withassertNotEq(ethereumForkId, bscForkId, "Fork IDs must differ")(and so on) to get clearer diagnostics showing which fork IDs collided.Apply this diff:
function testForkIdDiffer() public view { - assert(ethereumForkId != bscForkId); - assert(bscForkId != polygonForkId); - assert(polygonForkId != baseForkId); - assert(baseForkId != arbitrumForkId); - assert(arbitrumForkId != avalancheForkId); - assert(avalancheForkId != zetachainForkId); + assertNotEq(ethereumForkId, bscForkId, "Ethereum and BSC fork IDs must differ"); + assertNotEq(bscForkId, polygonForkId, "BSC and Polygon fork IDs must differ"); + assertNotEq(polygonForkId, baseForkId, "Polygon and Base fork IDs must differ"); + assertNotEq(baseForkId, arbitrumForkId, "Base and Arbitrum fork IDs must differ"); + assertNotEq(arbitrumForkId, avalancheForkId, "Arbitrum and Avalanche fork IDs must differ"); + assertNotEq(avalancheForkId, zetachainForkId, "Avalanche and ZetaChain fork IDs must differ"); }
75-80: Consider renaming for clarity.The function name
testUpgradeGatewayOnAllChainsis specific to Gateway contracts, but this is a base class that can test any upgradable contract (as evidenced byERC20CustodyForkTestalso using this base). Consider renaming totestUpgradeContractOnAllChainsfor better clarity.
45-55: Refactor fork creation to avoid requiring all RPC env vars
Eagerly callingvm.createForkfor every supported chain insetUp()forces all sevenRPC_URLenv vars to be set, even if a subclass only needs a subset. Move thosecreateForkcalls into_setupChains()or guard against empty URLs so tests can opt into only the networks they actually use.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/fork/BaseForkTest.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/fork/BaseForkTest.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: generate
- GitHub Check: slither
- GitHub Check: test
There was a problem hiding this comment.
We can now remove these scripts right? https://github.com/zeta-chain/protocol-contracts-evm/tree/develop/scripts/upgrade
| uint256 avalancheForkId; | ||
| uint256 zetachainForkId; | ||
|
|
||
| // Mainnet admins. |
There was a problem hiding this comment.
The test only work for mainnet? We should precise it somewhere. Otherwise can't we make it dependent on the network used?
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1)
47-56: Action: Align withdraw() Natspec with actual role check (TSS vs WITHDRAWER_ROLE).Natspec says "only TSS address" but the function uses onlyRole(WITHDRAWER_ROLE). Update the @dev to mention WITHDRAWER_ROLE or change the modifier to onlyRole(TSS_ROLE) depending on intended authority. File: test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (withdraw()).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (32)
contracts/evm/ERC20Custody.sol(2 hunks)contracts/evm/GatewayEVM.sol(6 hunks)contracts/evm/Registry.sol(7 hunks)contracts/evm/ZetaConnectorBase.sol(1 hunks)contracts/evm/ZetaConnectorNative.sol(1 hunks)contracts/evm/ZetaConnectorNonNative.sol(2 hunks)contracts/evm/interfaces/IGatewayEVM.sol(3 hunks)contracts/evm/legacy/ZetaConnector.base.sol(2 hunks)contracts/evm/legacy/ZetaConnector.eth.sol(3 hunks)contracts/evm/legacy/ZetaConnector.non-eth.sol(3 hunks)contracts/evm/legacy/ZetaInterfaces.sol(1 hunks)contracts/helpers/BaseRegistry.sol(3 hunks)contracts/helpers/interfaces/IBaseRegistry.sol(3 hunks)contracts/zevm/CoreRegistry.sol(7 hunks)contracts/zevm/GatewayZEVM.sol(5 hunks)contracts/zevm/ZRC20.sol(1 hunks)contracts/zevm/interfaces/IGatewayZEVM.sol(3 hunks)contracts/zevm/interfaces/UniversalContract.sol(2 hunks)contracts/zevm/legacy/ZetaConnectorZEVM.sol(3 hunks)test/CoreRegistry.t.sol(1 hunks)test/ERC20Custody.t.sol(3 hunks)test/GatewayEVM.t.sol(1 hunks)test/GatewayEVMZEVM.t.sol(1 hunks)test/GatewayZEVM.t.sol(2 hunks)test/Registry.t.sol(1 hunks)test/ZRC20.t.sol(1 hunks)test/utils/ReceiverEVM.sol(1 hunks)test/utils/upgrades/ERC20CustodyUpgradeTest.sol(2 hunks)test/utils/upgrades/GatewayEVMUpgradeTest.sol(7 hunks)test/utils/upgrades/GatewayZEVMUpgradeTest.sol(4 hunks)test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol(1 hunks)test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol(2 hunks)
✅ Files skipped from review due to trivial changes (25)
- contracts/evm/interfaces/IGatewayEVM.sol
- contracts/zevm/legacy/ZetaConnectorZEVM.sol
- contracts/evm/ZetaConnectorBase.sol
- contracts/zevm/CoreRegistry.sol
- test/GatewayEVMZEVM.t.sol
- test/CoreRegistry.t.sol
- contracts/evm/ERC20Custody.sol
- contracts/zevm/ZRC20.sol
- test/Registry.t.sol
- test/ZRC20.t.sol
- contracts/evm/legacy/ZetaConnector.non-eth.sol
- contracts/evm/ZetaConnectorNonNative.sol
- contracts/zevm/GatewayZEVM.sol
- contracts/evm/ZetaConnectorNative.sol
- contracts/evm/legacy/ZetaConnector.base.sol
- test/ERC20Custody.t.sol
- test/utils/upgrades/ERC20CustodyUpgradeTest.sol
- test/GatewayZEVM.t.sol
- test/GatewayEVM.t.sol
- contracts/evm/legacy/ZetaConnector.eth.sol
- contracts/helpers/interfaces/IBaseRegistry.sol
- test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol
- contracts/zevm/interfaces/IGatewayZEVM.sol
- contracts/zevm/interfaces/UniversalContract.sol
- test/utils/upgrades/GatewayEVMUpgradeTest.sol
🧰 Additional context used
📓 Path-based instructions (2)
contracts/**
⚙️ CodeRabbit configuration file
Review the Solidity contracts for security vulnerabilities and best practices.
Files:
contracts/helpers/BaseRegistry.solcontracts/evm/Registry.solcontracts/evm/legacy/ZetaInterfaces.solcontracts/evm/GatewayEVM.sol
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/utils/ReceiverEVM.soltest/utils/upgrades/GatewayZEVMUpgradeTest.soltest/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: generate
- GitHub Check: slither
🔇 Additional comments (10)
test/utils/ReceiverEVM.sol (1)
76-83: LGTM! Formatting change only.The function signature has been consolidated to a single line without any functional changes.
contracts/helpers/BaseRegistry.sol (1)
284-284: LGTM! Formatting changes align with PR-wide standardization.These function declarations have been reformatted from multiline to single-line signatures without any semantic changes. The modifications are consistent with the broader formatting effort across the PR and introduce no security, correctness, or behavioral issues.
Also applies to: 298-298, 339-339
contracts/evm/Registry.sol (1)
35-35: LGTM! Formatting changes improve consistency.The function declarations have been reformatted from multi-line to single-line forms without altering signatures, visibility, or behavior. This is consistent with the broader formatting standardization effort across the codebase.
Also applies to: 65-65, 96-96, 110-110, 124-124, 200-200, 222-222
contracts/evm/legacy/ZetaInterfaces.sol (1)
96-98: Formatting-only change; ABI/signature unchanged — OK.Params, visibility, and return type are identical; selector unaffected. Good to merge.
contracts/evm/GatewayEVM.sol (1)
106-106: LGTM: Formatting consolidation improves consistency.The function signature formatting changes (multi-line to single-line) are purely cosmetic and don't alter parameter types, names, visibility, or modifiers. The consolidation improves code consistency across the contract.
Also applies to: 127-127, 246-246, 263-263, 308-308, 426-426
test/utils/upgrades/GatewayZEVMUpgradeTest.sol (4)
149-171: Formatting-only change; behavior unchanged. Confirm event rename scope.Signature reflow looks good. Also, only this ZRC20 withdraw emits WithdrawnV2 while other withdraw/withdrawAndCall still emit Withdrawn—confirm this asymmetry is intentional for the upgrade test.
255-277: LGTM: single-line signature reflow.No ABI/behavior change introduced.
475-486: LGTM: signature reflow only.No logic changes.
503-513: LGTM: signature reflow only.No logic changes.
test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1)
24-32: LGTM: initialize signature reflow.No functional change.
…grades # Conflicts: # contracts/evm/GatewayEVM.sol # test/utils/upgrades/GatewayEVMUpgradeTest.sol
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/evm/interfaces/IGatewayEVM.sol (1)
165-167: Consider standardizing function signature formatting across the interface.The formatting consolidation creates an inconsistency within this interface file. Some function declarations now use single-line format (e.g.,
executeRevert,execute,depositAndCall), while others remain multi-line (e.g.,executeWithERC20,revertWithERC20, other overloads ofdeposit/depositAndCall, andcall).For better readability and maintainability, consider applying consistent formatting throughout the interface—either single-line for all or multi-line for all.
Also applies to: 175-178, 224-226
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
pkg/baseforktest.sol/baseforktest.gois excluded by!pkg/**pkg/erc20custodyfork.t.sol/erc20custodyforktest.gois excluded by!pkg/**pkg/gatewayevmfork.t.sol/gatewayevmforktest.gois excluded by!pkg/**pkg/gatewayzevmfork.t.sol/gatewayzevmforktest.gois excluded by!pkg/**types/BaseForkTest.tsis excluded by!types/**types/factories/BaseForkTest__factory.tsis excluded by!types/**
📒 Files selected for processing (9)
contracts/evm/Registry.sol(7 hunks)contracts/evm/ZetaConnectorNative.sol(1 hunks)contracts/evm/interfaces/IGatewayEVM.sol(3 hunks)contracts/helpers/BaseRegistry.sol(3 hunks)contracts/helpers/interfaces/IBaseRegistry.sol(3 hunks)contracts/zevm/CoreRegistry.sol(7 hunks)contracts/zevm/GatewayZEVM.sol(5 hunks)contracts/zevm/interfaces/UniversalContract.sol(2 hunks)test/utils/upgrades/GatewayZEVMUpgradeTest.sol(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- contracts/zevm/GatewayZEVM.sol
- contracts/evm/Registry.sol
- test/utils/upgrades/GatewayZEVMUpgradeTest.sol
- contracts/evm/ZetaConnectorNative.sol
- contracts/helpers/BaseRegistry.sol
🧰 Additional context used
📓 Path-based instructions (1)
contracts/**
⚙️ CodeRabbit configuration file
Review the Solidity contracts for security vulnerabilities and best practices.
Files:
contracts/helpers/interfaces/IBaseRegistry.solcontracts/zevm/interfaces/UniversalContract.solcontracts/evm/interfaces/IGatewayEVM.solcontracts/zevm/CoreRegistry.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: slither
- GitHub Check: generate
🔇 Additional comments (3)
contracts/zevm/interfaces/UniversalContract.sol (1)
21-21: LGTM - Formatting consolidation.The function declarations have been reformatted from multi-line to single-line format with no semantic changes.
Also applies to: 69-70
contracts/helpers/interfaces/IBaseRegistry.sol (1)
285-288: LGTM - Formatting consolidation.The interface function declarations have been reformatted from multi-line to single-line format with no changes to signatures or behavior.
Also applies to: 295-298, 324-327
contracts/zevm/CoreRegistry.sol (1)
51-54: LGTM - Formatting consolidation.All function declarations have been reformatted from multi-line to single-line format with no changes to parameters, modifiers, or logic.
Also applies to: 67-70, 83-86, 121-124, 160-160, 173-174, 197-198
|
@lumtis When I asked for removing tests, I meant this: https://github.com/zeta-chain/protocol-contracts-evm/tree/develop/test/utils/upgrades |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/fork/ERC20CustodyFork.t.sol (1)
60-64: Admin preservation check added — resolves earlier feedback.Good addition verifying DEFAULT_ADMIN_ROLE remains after upgrade.
🧹 Nitpick comments (5)
test/fork/ERC20CustodyFork.t.sol (5)
37-41: Capture a few more invariants to better cover storage layout.If available on ERC20Custody, also snapshot: paused state, role memberships (e.g., UPGRADER_ROLE), any limits/caps, and a representative mapping value (if feasible). This increases sensitivity to layout shifts per Issue #571.
43-48: Assert upgrade authorization and Upgraded event; allow optional migration call.
- Guard: expect revert if caller isn’t authorized (if configs change).
- Assert EIP-1967 Upgraded(address) is emitted.
- If the new impl requires a reinitializer, pass data via upgradeToAndCall.
- vm.startPrank(config.admin); + vm.startPrank(config.admin); + vm.recordLogs(); ERC20Custody localNewImplementation = new ERC20Custody(); - custody.upgradeToAndCall(address(localNewImplementation), ""); + // If a migration is needed in future, pass encoded call here instead of "". + custody.upgradeToAndCall(address(localNewImplementation), ""); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool sawUpgraded; + for (uint256 i; i < logs.length; ++i) { + // keccak256("Upgraded(address)") + if (logs[i].topics.length > 0 && logs[i].topics[0] == 0x5bfa8...000) { sawUpgraded = true; break; } + } + assertTrue(sawUpgraded, "Upgraded event not found");Note: replace 0x5bfa8... with the exact topic keccak256("Upgraded(address)").
50-57: Verify new implementation code is non-empty and optionally ERC1822 proxiableUUID.Extra safety so we don’t “upgrade” to a non-contract or wrong impl.
address onChainNewImplementation = _getImplementation(address(custody)); assertEq(onChainNewImplementation, address(localNewImplementation), "Upgrade failed."); + require(onChainNewImplementation.code.length > 0, "Impl has no code"); + // Optional (UUPS): sanity-check ERC1822 UUID if available + // bytes32 uuid = custody.proxiableUUID(); + // assertEq(uuid, 0x1822... , "Unexpected UUPS UUID");
55-57: Consider pre-validating that pre-upgrade values are sane (non-zero) to avoid false positives.E.g., require(gatewayBefore != address(0)) so “unchanged” doesn’t pass when state was already empty due to wrong proxy.
1-2: CI/ops: decide on RPC sourcing and concurrency to stabilize fork tests.
- Source RPCs from foundry.toml aliases with env overrides; prefer public endpoints for CI, private for local.
- Limit forks per CI run or shard tests by chain to avoid rate limits/timeouts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
data/addresses.testnet.jsonis excluded by!data/**pkg/erc20custodyfork.t.sol/erc20custodyforktest.gois excluded by!pkg/**pkg/gatewayevmfork.t.sol/gatewayevmforktest.gois excluded by!pkg/**pkg/gatewayzevmfork.t.sol/gatewayzevmforktest.gois excluded by!pkg/**types/factories/index.tsis excluded by!types/**types/index.tsis excluded by!types/**
📒 Files selected for processing (6)
scripts/upgrade/SimulateERC20CustodyUpgrade.s.sol(0 hunks)scripts/upgrade/SimulateGatewayEVMUpgrade.s.sol(0 hunks)scripts/upgrade/SimulateGatewayZEVMUpgrade.s.sol(0 hunks)test/fork/ERC20CustodyFork.t.sol(1 hunks)test/fork/GatewayEVMFork.t.sol(1 hunks)test/fork/GatewayZEVMFork.t.sol(1 hunks)
💤 Files with no reviewable changes (3)
- scripts/upgrade/SimulateERC20CustodyUpgrade.s.sol
- scripts/upgrade/SimulateGatewayZEVMUpgrade.s.sol
- scripts/upgrade/SimulateGatewayEVMUpgrade.s.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- test/fork/GatewayZEVMFork.t.sol
- test/fork/GatewayEVMFork.t.sol
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/fork/ERC20CustodyFork.t.sol
🪛 GitHub Actions: Test
test/fork/ERC20CustodyFork.t.sol
[error] 1-1: Fork test failed: vm.envString: environment variable "ETHEREUM_RPC_URL" not found. Test ERC20CustodyForkTest constructors aborted. Command 'yarn test' exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: slither
- GitHub Check: generate
🔇 Additional comments (1)
test/fork/ERC20CustodyFork.t.sol (1)
8-15: The hardcoded proxy address constants are not placeholders—they are the correct on-chain addresses from deployment records. Verify indata/addresses.mainnet.jsonanddata/checksum/mainnet.json: Ethereum/BSC/Polygon/Base share0x0Bad...and Arbitrum/Avalanche share0xECe3...due to CREATE2 determinism with identical salts, confirmed by the broadcast record atbroadcast/DeployERC20Custody.s.sol/1/run-latest.jsonwhere the proxy was deployed to0x0Bad40D9e9C369f2223c835E108f43a45fd223B5on Ethereum.The test will execute correctly if RPC URLs and environment variables are properly configured. The defensive suggestions (adding code presence checks) remain reasonable best practices but are not required fixes.
Likely an incorrect or invalid review comment.
| function _setupChains() internal override { | ||
| chains.push( | ||
| ChainConfig(ethereumForkId, ETHEREUM_ERC20CUSTODY_PROXY, ETHEREUM_ADMIN, ETHEREUM_RPC_URL, "Ethereum") | ||
| ); | ||
| chains.push(ChainConfig(bscForkId, BSC_ERC20CUSTODY_PROXY, BSC_ADMIN, BSC_RPC_URL, "BSC")); | ||
| chains.push(ChainConfig(polygonForkId, POLYGON_ERC20CUSTODY_PROXY, POLYGON_ADMIN, POLYGON_RPC_URL, "Polygon")); | ||
| chains.push(ChainConfig(baseForkId, BASE_ERC20CUSTODY_PROXY, BASE_ADMIN, BASE_RPC_URL, "Base")); | ||
| chains.push( | ||
| ChainConfig(arbitrumForkId, ARBITRUM_ERC20CUSTODY_PROXY, ARBITRUM_ADMIN, ARBITRUM_RPC_URL, "Arbitrum") | ||
| ); | ||
| chains.push( | ||
| ChainConfig(avalancheForkId, AVALANCHE_ERC20CUSTODY_PROXY, AVALANCHE_ADMIN, AVALANCHE_RPC_URL, "Avalanche") | ||
| ); | ||
| } |
There was a problem hiding this comment.
CI failure: missing RPC envs. Add robust fallbacks and gating to avoid constructor aborts.
Current pipeline error: vm.envString "ETHEREUM_RPC_URL" not found. Provide defaults via foundry aliases/public RPCs and skip chains without URLs.
Action (in BaseForkTest.sol; outside this file):
- Load RPCs with fallbacks to foundry.toml aliases and safe public endpoints; include gating so chains with empty URLs are not pushed.
Example (BaseForkTest.setUp):
// Foundry cheatcodes allow fallbacks
string memory ethUrl = vm.envOr("ETHEREUM_RPC_URL", vm.rpcUrlOr("ethereum", "https://ethereum.publicnode.com"));
string memory bscUrl = vm.envOr("BSC_RPC_URL", vm.rpcUrlOr("bsc", "https://bsc.publicnode.com"));
string memory polUrl = vm.envOr("POLYGON_RPC_URL", vm.rpcUrlOr("polygon", "https://polygon-bor.publicnode.com"));
string memory baseUrl= vm.envOr("BASE_RPC_URL", vm.rpcUrlOr("base", "https://base.publicnode.com"));
string memory arbUrl = vm.envOr("ARBITRUM_RPC_URL", vm.rpcUrlOr("arbitrum", "https://arbitrum-one.publicnode.com"));
string memory avaUrl = vm.envOr("AVALANCHE_RPC_URL",vm.rpcUrlOr("avalanche","https://avalanche-c-chain.publicnode.com"));
// Only create forks/push configs when URL is non-empty
if (bytes(ethUrl).length != 0) { ethereumForkId = vm.createFork(ethUrl); /* push chain config */ }Optional gating in CI: require RUN_FORK_TESTS=true; otherwise skip:
bool runForks = vm.envOr("RUN_FORK_TESTS", false);
if (!runForks) return; // do not push any chainsAlso add these to foundry.toml:
[rpc_endpoints]
ethereum = "${ETHEREUM_RPC_URL}"
bsc = "${BSC_RPC_URL}"
polygon = "${POLYGON_RPC_URL}"
base = "${BASE_RPC_URL}"
arbitrum = "${ARBITRUM_RPC_URL}"
avalanche= "${AVALANCHE_RPC_URL}"
🤖 Prompt for AI Agents
In test/fork/ERC20CustodyFork.t.sol around lines 16-29 the test pushes chain
configs unconditionally which causes CI failures when RPC env vars are missing;
update BaseForkTest.setUp (not this file) to load RPC URLs with robust fallbacks
using vm.envOr and vm.rpcUrlOr (or safe public endpoints) for each chain, only
create forks and push ChainConfig entries when the resolved URL is non-empty,
and add an optional RUN_FORK_TESTS gating (vm.envOr) to skip all fork setup when
not enabled; also update foundry.toml rpc_endpoints to reference the env vars so
vm.rpcUrlOr can resolve aliases.
| function _testUpgradeContract(ChainConfig memory config) internal override { | ||
| vm.selectFork(config.forkId); | ||
|
|
||
| // Get the current proxy contract. | ||
| ERC20Custody custody = ERC20Custody(config.contractAddress); | ||
|
|
There was a problem hiding this comment.
Strengthen preconditions on the fork and proxy before upgrade.
Add sanity checks to fail fast with actionable messages.
vm.selectFork(config.forkId);
// Get the current proxy contract.
- ERC20Custody custody = ERC20Custody(config.contractAddress);
+ require(config.forkId == vm.activeFork(), "Fork not active");
+ require(config.contractAddress.code.length > 0, "No code at proxy");
+ ERC20Custody custody = ERC20Custody(config.contractAddress);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _testUpgradeContract(ChainConfig memory config) internal override { | |
| vm.selectFork(config.forkId); | |
| // Get the current proxy contract. | |
| ERC20Custody custody = ERC20Custody(config.contractAddress); | |
| function _testUpgradeContract(ChainConfig memory config) internal override { | |
| vm.selectFork(config.forkId); | |
| // Get the current proxy contract. | |
| require(config.forkId == vm.activeFork(), "Fork not active"); | |
| require(config.contractAddress.code.length > 0, "No code at proxy"); | |
| ERC20Custody custody = ERC20Custody(config.contractAddress); |
🤖 Prompt for AI Agents
In test/fork/ERC20CustodyFork.t.sol around lines 31 to 36, add fast-fail sanity
checks: validate config.forkId is non-zero before selecting the fork, then after
vm.selectFork ensure config.contractAddress is not address(0) and that the
target address has on-chain code (address(config.contractAddress).code.length >
0) before casting to ERC20Custody; use require/assert with clear, actionable
error messages for each check so tests fail quickly with guidance.
Closes: #571
Summary by CodeRabbit
Tests
Chores